fix(models/ocr): use OS trust store for model downloads; route polars via lts-cpu wrapper#1175
Open
ksaurabhAparavi wants to merge 1 commit into
Conversation
ksaurabhAparavi
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
Contributor
📝 WalkthroughWalkthroughThis PR adds import-time fixes: an SSL trust-store patcher used by the model loader and a polars-lts-cpu wrapper used by the OCR node, plus platform-conditional requirements to ensure correct binaries are installed on x86_64 hosts. ChangesTLS/SSL and Polars LTS CPU Runtime Robustness
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ai/src/ai/common/ssl/__init__.py`:
- Around line 41-42: The call to depends(requirements) is executed before the
fallback try/except and can raise during import; move the depends(requirements)
invocation into the existing try block that attempts the truststore installation
so failures are caught by the except and the module can fall back to certifi or
the default SSL context. Specifically, relocate the depends(requirements) call
so it runs inside the same try that wraps truststore installation (referencing
the requirements variable and the truststore installation logic) and do not call
depends at module import level outside that try/except.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aca16ad0-7964-4b0a-9f1f-ab329687d4fd
📒 Files selected for processing (7)
nodes/src/nodes/ocr/IGlobal.pynodes/src/nodes/ocr/requirements.txtpackages/ai/src/ai/common/models/__init__.pypackages/ai/src/ai/common/polars/__init__.pypackages/ai/src/ai/common/polars/requirements.txtpackages/ai/src/ai/common/ssl/__init__.pypackages/ai/src/ai/common/ssl/requirements.txt
Comment on lines
+41
to
+42
| requirements = os.path.dirname(os.path.realpath(__file__)) + '/requirements.txt' | ||
| depends(requirements) |
Contributor
There was a problem hiding this comment.
Move depends(requirements) inside the fallback try.
Lines 41-42 can raise before the fallback logic starts, so a failed truststore install aborts ai.common.ssl import entirely instead of degrading to certifi or leaving the default SSL context unchanged. Because packages/ai/src/ai/common/models/__init__.py imports this module eagerly, that turns a transient pip/network failure into a hard failure for every ai.common.models import.
Suggested fix
-requirements = os.path.dirname(os.path.realpath(__file__)) + '/requirements.txt'
-depends(requirements)
-
try:
+ requirements = os.path.dirname(os.path.realpath(__file__)) + '/requirements.txt'
+ depends(requirements)
import truststore
truststore.inject_into_ssl()
except Exception:Based on learnings, import-time depends(...) in packages/ai/src/ai/**/__init__.py is intentional; the problem here is only that this call sits outside the graceful-fallback path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| requirements = os.path.dirname(os.path.realpath(__file__)) + '/requirements.txt' | |
| depends(requirements) | |
| try: | |
| requirements = os.path.dirname(os.path.realpath(__file__)) + '/requirements.txt' | |
| depends(requirements) | |
| import truststore | |
| truststore.inject_into_ssl() | |
| except Exception: | |
| # fallback logic continues here |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ai/src/ai/common/ssl/__init__.py` around lines 41 - 42, The call to
depends(requirements) is executed before the fallback try/except and can raise
during import; move the depends(requirements) invocation into the existing try
block that attempts the truststore installation so failures are caught by the
except and the module can fall back to certifi or the default SSL context.
Specifically, relocate the depends(requirements) call so it runs inside the same
try that wraps truststore installation (referencing the requirements variable
and the truststore installation logic) and do not call depends at module import
level outside that try/except.
Source: Learnings
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID. Do not edit or delete. |
github-actions
Bot
added
module:nodes
… via lts-cpu wrapper - ssl: install truststore and inject it into the default SSL context so HTTPS model downloads use the full OS trust store, fixing "unable to get local issuer certificate" behind TLS-intercepting proxies (falls back to certifi). - polars: route OCR polars imports through ai.common.polars, which installs polars-lts-cpu and removes any transitively-pulled plain polars, fixing SIGILL / SEH 0xc000001d on x86_64 hosts without AVX2/FMA. Fixes rocketride-org#1162
ksaurabhAparavi
force-pushed
the
fix/RR-1162-model-download-tls-and-polars
branch
from
d5c6abc to
a940903
Compare
ksaurabhAparavi
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ai/src/ai/common/polars/__init__.py`:
- Around line 51-77: The current x86_64 remediation block (guarded by
_NEEDS_LTS) swallows all exceptions and ignores pip(...) return values, so
uninstall/install failures can leave an incompatible polars present; change the
cleanup to fail fast: after each pip('uninstall'...) and pip('install'...) call
in the remediation branch, check the boolean return and if False raise a clear
RuntimeError (or re-raise the underlying exception) so the import fails fast;
also avoid the blanket except Exception: pass — either remove it or re-raise
after logging so failures in the remediation (within the try around
importlib.metadata, pip calls, or sys.modules manipulations) surface immediately
and prevent a silent fallback to an incompatible polars.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb20251c-e4ff-4c05-ab8f-d24ec4be900a
📒 Files selected for processing (7)
nodes/src/nodes/ocr/IGlobal.pynodes/src/nodes/ocr/requirements.txtpackages/ai/src/ai/common/models/__init__.pypackages/ai/src/ai/common/polars/__init__.pypackages/ai/src/ai/common/polars/requirements.txtpackages/ai/src/ai/common/ssl/__init__.pypackages/ai/src/ai/common/ssl/requirements.txt
Comment on lines
+51
to
+77
| if _NEEDS_LTS: | ||
| try: | ||
| import importlib.metadata as _md | ||
|
|
||
| _has_plain_polars = False | ||
| try: | ||
| _md.version('polars') | ||
| _has_plain_polars = True | ||
| except _md.PackageNotFoundError: | ||
| # Plain `polars` not installed — only polars-lts-cpu is on disk, | ||
| # which is exactly the desired state. No cleanup needed. | ||
| pass | ||
|
|
||
| if _has_plain_polars: | ||
| # Plain `polars` was pulled in transitively (img2table etc.). | ||
| # Drop it and force-reinstall lts-cpu so its binary wins on disk. | ||
| pip('uninstall', '-y', 'polars') | ||
| pip('install', '--force-reinstall', '--no-deps', 'polars-lts-cpu') | ||
|
|
||
| # Drop any already-loaded polars modules so the next import | ||
| # picks up the freshly-written files instead of cached state. | ||
| for _mod in [m for m in list(sys.modules) if m == 'polars' or m.startswith('polars.')]: | ||
| sys.modules.pop(_mod, None) | ||
| except Exception: | ||
| # Best-effort cleanup. If it fails, the import below will surface | ||
| # the underlying issue with a real traceback. | ||
| pass |
Contributor
There was a problem hiding this comment.
Fail fast when polars remediation does not succeed.
The x86_64 remediation path treats uninstall/reinstall as best-effort and swallows failures. Because pip(...) returns a boolean, a failed cleanup can silently leave incompatible polars active, which reintroduces the crash class this module is meant to prevent.
Suggested fix
if _NEEDS_LTS:
try:
import importlib.metadata as _md
@@
if _has_plain_polars:
@@
- pip('uninstall', '-y', 'polars')
- pip('install', '--force-reinstall', '--no-deps', 'polars-lts-cpu')
+ uninstalled = pip('uninstall', '-y', 'polars')
+ installed = pip('install', '--force-reinstall', '--no-deps', 'polars-lts-cpu')
+ if not (uninstalled and installed):
+ raise RuntimeError('Failed to enforce polars-lts-cpu on x86_64 host')
@@
- except Exception:
- # Best-effort cleanup. If it fails, the import below will surface
- # the underlying issue with a real traceback.
- pass
+ except Exception as exc:
+ raise RuntimeError('Polars runtime remediation failed before import') from exc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if _NEEDS_LTS: | |
| try: | |
| import importlib.metadata as _md | |
| _has_plain_polars = False | |
| try: | |
| _md.version('polars') | |
| _has_plain_polars = True | |
| except _md.PackageNotFoundError: | |
| # Plain `polars` not installed — only polars-lts-cpu is on disk, | |
| # which is exactly the desired state. No cleanup needed. | |
| pass | |
| if _has_plain_polars: | |
| # Plain `polars` was pulled in transitively (img2table etc.). | |
| # Drop it and force-reinstall lts-cpu so its binary wins on disk. | |
| pip('uninstall', '-y', 'polars') | |
| pip('install', '--force-reinstall', '--no-deps', 'polars-lts-cpu') | |
| # Drop any already-loaded polars modules so the next import | |
| # picks up the freshly-written files instead of cached state. | |
| for _mod in [m for m in list(sys.modules) if m == 'polars' or m.startswith('polars.')]: | |
| sys.modules.pop(_mod, None) | |
| except Exception: | |
| # Best-effort cleanup. If it fails, the import below will surface | |
| # the underlying issue with a real traceback. | |
| pass | |
| if _NEEDS_LTS: | |
| try: | |
| import importlib.metadata as _md | |
| _has_plain_polars = False | |
| try: | |
| _md.version('polars') | |
| _has_plain_polars = True | |
| except _md.PackageNotFoundError: | |
| # Plain `polars` not installed — only polars-lts-cpu is on disk, | |
| # which is exactly the desired state. No cleanup needed. | |
| pass | |
| if _has_plain_polars: | |
| # Plain `polars` was pulled in transitively (img2table etc.). | |
| # Drop it and force-reinstall lts-cpu so its binary wins on disk. | |
| uninstalled = pip('uninstall', '-y', 'polars') | |
| installed = pip('install', '--force-reinstall', '--no-deps', 'polars-lts-cpu') | |
| if not (uninstalled and installed): | |
| raise RuntimeError('Failed to enforce polars-lts-cpu on x86_64 host') | |
| # Drop any already-loaded polars modules so the next import | |
| # picks up the freshly-written files instead of cached state. | |
| for _mod in [m for m in list(sys.modules) if m == 'polars' or m.startswith('polars.')]: | |
| sys.modules.pop(_mod, None) | |
| except Exception as exc: | |
| raise RuntimeError('Polars runtime remediation failed before import') from exc |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ai/src/ai/common/polars/__init__.py` around lines 51 - 77, The
current x86_64 remediation block (guarded by _NEEDS_LTS) swallows all exceptions
and ignores pip(...) return values, so uninstall/install failures can leave an
incompatible polars present; change the cleanup to fail fast: after each
pip('uninstall'...) and pip('install'...) call in the remediation branch, check
the boolean return and if False raise a clear RuntimeError (or re-raise the
underlying exception) so the import fails fast; also avoid the blanket except
Exception: pass — either remove it or re-raise after logging so failures in the
remediation (within the try around importlib.metadata, pip calls, or sys.modules
manipulations) surface immediately and prevent a silent fallback to an
incompatible polars.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
truststoreand inject it into the default SSL context so HTTPS model downloads use the full OS trust store — fixes "unable to get local issuer certificate" behind TLS-intercepting proxies (falls back tocertifi).ai.common.polars, which installspolars-lts-cpuand removes any transitively-pulled plainpolars, fixing SIGILL / SEH0xc000001don x86_64 hosts without AVX2/FMA.Testing
./builder test) — relying on GitHub Actions; not runnable in the contributor's local shell (engine build / Maven / torch unavailable). Static checks (compile, no conflict markers) pass.Linked Issue
Fixes #1162